feat: Added warning when failing to update index cache #15014

ranger-ross · 2025-01-04T03:49:57Z

What does this PR try to resolve?

Adds a warning if there is an error updating the index cache.
It also attempts to avoid warning spam as requested in this comment

Below is an example output

    Updating crates.io index
warning: failed to write cache, path: /home/ross/.cargo/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/ba/se/base64, error: Permission denied (os error 13)
   Compiling serde v1.0.217
   Compiling r-efi v5.2.0
   Compiling base64 v0.22.1
   Compiling cargo-13712 v0.1.0 (/home/ross/projects/cargo-13712)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 1.20s

How should we test and review this PR?

I tested this on my machine by manually changing the owner/permission of the index files

sudo chown root ~/.cargo/registry/index/.../.cache/r-
sudo chmod 700 ~/.cargo/registry/index/.../.cache/r-

# in a project that uses the `r-efi` crate
cargo build

I wasn't quiet sure about the best way to add a test to the testsuite for this. 😅
If there is good way to create a test for this lmk

rustbot · 2025-01-04T03:50:03Z

r? @weihanglo

rustbot has assigned @weihanglo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

src/cargo/sources/registry/index/cache.rs

weihanglo · 2025-01-07T21:19:53Z

src/cargo/sources/registry/index/cache.rs

We have a similar test, maybe we can take reuse that test to assert their stderr?

cargo/tests/testsuite/registry.rs

Lines 3053 to 3098 in 2939e96

fn readonly_registry_still_works() {

Package::new("foo", "0.1.0").publish();

let p = project()

.file(

"Cargo.toml",

r#"

[package]

name = "a"

version = "0.5.0"

edition = "2015"

authors = []

[dependencies]

foo = '0.1.0'

"#,

)

.file("src/lib.rs", "")

.build();

p.cargo("generate-lockfile").run();

p.cargo("fetch --locked").run();

chmod_readonly(&paths::home(), true);

p.cargo("check").run();

// make sure we un-readonly the files afterwards so "cargo clean" can remove them (#6934)

chmod_readonly(&paths::home(), false);

fn chmod_readonly(path: &Path, readonly: bool) {

for entry in t!(path.read_dir()) {

let entry = t!(entry);

let path = entry.path();

if t!(entry.file_type()).is_dir() {

chmod_readonly(&path, readonly);

} else {

set_readonly(&path, readonly);

}

}

set_readonly(path, readonly);

}

fn set_readonly(path: &Path, readonly: bool) {

let mut perms = t!(path.metadata()).permissions();

perms.set_readonly(readonly);

t!(fs::set_permissions(path, perms));

}

}

thanks, I was able to use this to create a test based off of this in 9f2d0a3

Shameless copied from #15026 (comment).

We generally ask that PRs are split into

One commit that adds the test, showing the currently broken behavior (ie it passes)

The fix commit that updates the test to pass with the change

This makes the diff between the two commits show the change in behavior

Mind doing that?

weihanglo · 2025-01-08T17:08:06Z

tests/testsuite/registry.rs

+#[cargo_test]
+#[cfg(not(windows))]


Wonder why it cannot be done on Windows. Is it possible to use Permissions::set_readonly here?

src/cargo/sources/registry/index/cache.rs

weihanglo · 2025-01-08T17:18:16Z

tests/testsuite/registry.rs

+                authors = []
+
+                [dependencies]
+                foo = '0.1.0'


Should we add more than one dependency, so we know it really emits only once?

rustbot assigned weihanglo Jan 4, 2025

rustbot added A-registries Area: registries S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2025

weihanglo reviewed Jan 7, 2025

View reviewed changes

feat: Added warning when failing to update index cache

08facee

ranger-ross force-pushed the cache-warnings branch from a50a73a to 9f2d0a3 Compare January 8, 2025 14:33

test: Added test to verify registry cache write error emit warnings

92ed0f1

ranger-ross force-pushed the cache-warnings branch from 9f2d0a3 to 92ed0f1 Compare January 8, 2025 15:04

weihanglo reviewed Jan 8, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Added warning when failing to update index cache #15014

feat: Added warning when failing to update index cache #15014

ranger-ross commented Jan 4, 2025 •

edited

Loading

rustbot commented Jan 4, 2025

weihanglo Jan 7, 2025

ranger-ross Jan 8, 2025

weihanglo Jan 8, 2025

weihanglo Jan 8, 2025

weihanglo Jan 8, 2025

	fn readonly_registry_still_works() {
	Package::new("foo", "0.1.0").publish();

	let p = project()
	.file(
	"Cargo.toml",
	r#"
	[package]
	name = "a"
	version = "0.5.0"
	edition = "2015"
	authors = []

	[dependencies]
	foo = '0.1.0'
	"#,
	)
	.file("src/lib.rs", "")
	.build();

	p.cargo("generate-lockfile").run();
	p.cargo("fetch --locked").run();
	chmod_readonly(&paths::home(), true);
	p.cargo("check").run();
	// make sure we un-readonly the files afterwards so "cargo clean" can remove them (#6934)
	chmod_readonly(&paths::home(), false);

	fn chmod_readonly(path: &Path, readonly: bool) {
	for entry in t!(path.read_dir()) {
	let entry = t!(entry);
	let path = entry.path();
	if t!(entry.file_type()).is_dir() {
	chmod_readonly(&path, readonly);
	} else {
	set_readonly(&path, readonly);
	}
	}
	set_readonly(path, readonly);
	}

	fn set_readonly(path: &Path, readonly: bool) {
	let mut perms = t!(path.metadata()).permissions();
	perms.set_readonly(readonly);
	t!(fs::set_permissions(path, perms));
	}
	}

		#[cargo_test]
		#[cfg(not(windows))]

feat: Added warning when failing to update index cache #15014

Are you sure you want to change the base?

feat: Added warning when failing to update index cache #15014

Conversation

ranger-ross commented Jan 4, 2025 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

rustbot commented Jan 4, 2025

weihanglo Jan 7, 2025

Choose a reason for hiding this comment

ranger-ross Jan 8, 2025

Choose a reason for hiding this comment

weihanglo Jan 8, 2025

Choose a reason for hiding this comment

weihanglo Jan 8, 2025

Choose a reason for hiding this comment

weihanglo Jan 8, 2025

Choose a reason for hiding this comment

ranger-ross commented Jan 4, 2025 •

edited

Loading